Skip to content
This repository has been archived by the owner on Jan 28, 2021. It is now read-only.

Give vitess a chance to enforce connection timeouts if we've been waiting for a row for longer #801

Closed
wants to merge 6 commits into from

Conversation

juanjux
Copy link
Contributor

@juanjux juanjux commented Aug 9, 2019

Fixes #799
Related issue: https://github.com/src-d/gitbase-spark-connector-enterprise/issues/81

This PR abort further rowIterator.Next() calls if the time waiting for a single row is bigger than the configured ConnReadTimeout thus giving a change to Vitess to call the Handler.CloseConnection() callback when it detects the timeout. It does it by running the calls to the iterator on a looped goroutine with an Timer which checks that we didn't exceed the timeout.

This doesn't fix issue #800 which implies detecting a closed connection before the timeout expires and killing the associated queries, but serves to somewhat workaround it by configuring a timeout.

Juanjo Alvarez added 2 commits August 9, 2019 18:26
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
@juanjux juanjux self-assigned this Aug 9, 2019
@juanjux juanjux changed the title Row timeout Give vitess a chance to enforce connection timeouts if we've been waiting for a row for longer Aug 9, 2019
server/handler.go Outdated Show resolved Hide resolved
server/handler.go Outdated Show resolved Hide resolved
server/handler.go Outdated Show resolved Hide resolved
server/handler.go Outdated Show resolved Hide resolved
server/handler.go Show resolved Hide resolved
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
server/handler.go Outdated Show resolved Hide resolved
server/handler.go Outdated Show resolved Hide resolved
server/handler.go Show resolved Hide resolved
…usage

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
server/handler.go Outdated Show resolved Hide resolved
server/handler.go Outdated Show resolved Hide resolved
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
server/handler.go Outdated Show resolved Hide resolved
server/handler.go Outdated Show resolved Hide resolved
server/handler.go Outdated Show resolved Hide resolved
server/handler.go Outdated Show resolved Hide resolved
server/handler.go Outdated Show resolved Hide resolved
server/handler.go Show resolved Hide resolved

rowchan := make(chan sql.Row)
errchan := make(chan error)
quit := make(chan struct{})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[optional] You could put defer close(quit) here, instead of calling it in multiple places below. As far as I can see, anytime this function returns, you'd want quit closed anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to make it explicit so the code reader can see where we are actively signaling the goroutine.

server/handler.go Show resolved Hide resolved
server/handler_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@agarciamontoro agarciamontoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (as far as I understand what the function does)

server/handler.go Outdated Show resolved Hide resolved
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

Refactor rowloop select

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
@juanjux
Copy link
Contributor Author

juanjux commented Aug 14, 2019

@ajnavarro dont merge, maybe my next PR will make this unnecesary.

lwsanty added a commit to src-d/regression-gitbase that referenced this pull request Aug 15, 2019
Signed-off-by: lwsanty <lwsanty@gmail.com>
@juanjux
Copy link
Contributor Author

juanjux commented Aug 15, 2019

Closing, definitely superseeded by the next PR that improves also this part and fixes a bug.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connection timeouts can't be processed if a query is running.
5 participants